proc: initial (local) process manager implementation #7048
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
pmrowla
added
skip-changelog
pmrowla
commented
Nov 25, 2021
Comment on lines
+66
to
+92
| def send_signal(self, name: str, signal: int): | ||
| """Send `signal` to the specified named process.""" | ||
| raise NotImplementedError | ||
|
|
||
| def kill(self, name: str): | ||
| """Kill the specified named process.""" | ||
| raise NotImplementedError | ||
|
|
||
| def terminate(self, name: str): | ||
| """Terminate the specified named process.""" | ||
| raise NotImplementedError | ||
|
|
||
| def remove(self, name: str, force: bool = False): | ||
| """Remove the specified named process from this manager. | ||
|
|
||
| If the specified process is still running, it will be forcefully killed | ||
| if `force` is True`, otherwise an exception will be raised. | ||
|
|
||
| Raises: | ||
| ProcessNotTerminatedError if the specified process is still | ||
| running and was not forcefully killed. | ||
| """ | ||
| raise NotImplementedError | ||
|
|
||
| def cleanup(self): | ||
| """Remove stale (terminated) processes from this manager.""" | ||
| raise NotImplementedError |
There was a problem hiding this comment.
@karajan1001 the next set of tasks will be to fill out these methods.
send_signal,terminate,killare all related and would be grouped together in the first task. Implementation/behavior for these methods should work the same as they do insubprocessandmultiprocessing(https://docs.python.org/3/library/subprocess.html#subprocess.Popen.send_signal).removeandcleanupcan be implemented onceterminate/killare done (since force-removing a child process directory will require killing the process if it is still running)
pmrowla
changed the title
pmrowla
force-pushed
the
process-manager
branch
from
2f4b768 to
0754aed
Compare
pmrowla
commented
Nov 25, 2021
| proc.start() | ||
| # Do not terminate the child daemon when the main process exits | ||
| # pylint: disable=protected-access | ||
| mp.process._children.discard(proc) # type: ignore[attr-defined] |
There was a problem hiding this comment.
This is kind of a hack, but even if we subclassed multiprocessing.Process we would just end up with an overridden
def start(self):
super().start()
_children.discard(self)
which I'm not sure is any cleaner
But in general, the idea behind using multiprocessing is that it allows us to use spawn instead of fork on windows + mac (#4294), and also allows us to directly run the target python function we want instead of adding the overhead from using dvc.daemon + a new dedicated CLI command
pmrowla
changed the title
pmrowla
commented
Nov 25, 2021
Comment on lines
+26
to
+41
| def __iter__(self): | ||
| return self.processes() | ||
|
|
||
| def __getitem__(self, key: str) -> "ProcessInfo": | ||
| info_path = os.path.join(self.wdir, key, f"{key}.json") | ||
| try: | ||
| with open(info_path, encoding="utf-8") as fobj: | ||
| return ProcessInfo.from_dict(json.load(fobj)) | ||
| except FileNotFoundError: | ||
| raise KeyError | ||
|
|
||
| def get(self, key: str, default=None): | ||
| try: | ||
| return self[key] | ||
| except KeyError: | ||
| return default |
There was a problem hiding this comment.
also in the future we probably want this to just mirror the standard dict/Mapping interface (and in processes()) so that we can return the paired name + info as needed instead of just the info from this initial implementation
skshetry
reviewed
Nov 25, 2021
skshetry
reviewed
Nov 25, 2021
pmrowla
force-pushed
the
process-manager
branch
from
0754aed to
2107127
Compare
- supports running (backgrounded) processes with redirected output
pmrowla
force-pushed
the
process-manager
branch
from
2107127 to
d69d9df
Compare
karajan1001
approved these changes
Nov 29, 2021
This was referenced Nov 29, 2021
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
❗ I have followed the Contributing to DVC checklist.
📖 If this PR requires documentation updates, I have created a separate PR (or issue, at least) in dvc.org and linked it here.
Thank you for the contribution - we'll try to review it as soon as possible. 🙏
Related to #6267
Local prerequisite for #7002
dvc.procmodule for managing background processesmultiprocessing.Process(daemon=True)Functionality is not used anywhere else in DVC yet (exp tempdir/queue runs will be the first place it gets added)
Python shell demo: